[step1] 문자열 계산기 1차 리뷰 요청드립니다! #3
Conversation
src/main/java/step1/main.java
Outdated
| @@ -0,0 +1,11 @@ | |||
| package step1; | |||
|
|
|||
| public class main { | |||
There was a problem hiding this comment.
클래스의 이름은 대문자로 시작하는 것이 컨벤션입니다 :)
| public class main { | |
| public class Main { |
| Print print = new Print(calculator.calculate()); | ||
| print.printResult(); | ||
| } | ||
| } |
There was a problem hiding this comment.
leeyohan93 13 minutes ago
파일 마지막에 엔터(개행문자)를 넣어주세요 :)
이유는 리뷰를 진행할 때 깃허브에서 경고메시지를 지우고 혹시 모르는 파일 읽기 오류에 대비하기 위함입니다.
좀 더 알고싶으시면 참고 링크를 보셔도 재밌을 것 같네요 :)
Intellij 를 사용하실 경우엔Preferences -> Editor -> General -> Ensure line feed at file end on save 를 체크해주시면파일 저장 시 마지막에 개행문자를 자동으로 넣어줍니다!
| private int calculateResult; | ||
|
|
||
| public Print(int calculateResult) { | ||
| this.calculateResult = calculateResult; | ||
| } |
There was a problem hiding this comment.
Print가 하나의 계산결과를 상태로 가지도록 구현하셨네요!
만약 Print를 통해 또 다른 계산 결과를 출력해야한다면 어떻게 될까요? Print라는 클래스의 책임에 맞는 역할을 하고 있다고 볼 수 있을까요?
| if (operatorList.get(operatorListIndex).equals("+")) { | ||
| result = result + (numberList.get(i)); | ||
| } else if (operatorList.get(operatorListIndex).equals("-")) { | ||
| result = result - (numberList.get(i)); | ||
| } else if (operatorList.get(operatorListIndex).equals("*")) { | ||
| result = result * (numberList.get(i)); | ||
| } else if (operatorList.get(operatorListIndex).equals("/")) { | ||
| result = result / (numberList.get(i)); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
java의 Enum 클래스를 사용하면 상태와 행위를 한곳에서 관리할 수 있는데요!
링크를 보시고 참고하셔서 enum 클래스를 사용해보면 좋을 것 같습니다 :)
| return result; | ||
| } | ||
|
|
||
| private int getResult(List<Integer> numberList, List<String> operatorList, int operatorListIndex, int result, int i) { |
There was a problem hiding this comment.
메서드의 파라미터의 개수를 2개 이하로 리팩터링해보는건 어떨까요?
클린 코드 책에서는 메서드의 파라미터 개수는 0~2개가 적절하며 4개 이상은 무조건적으로 피해야한다고 이야기합니다.
파라미터의 개수가 많을 수록 하나의 메서드에서 하는 일이 많아질 수 있고 코드를 읽기 어려워지기 때문입니다 :)
| private CalculatorResource calculatorResource; | ||
|
|
||
| public Calculator(String stringCalculatorSentence) { | ||
| this.calculatorResource = new CalculatorResource(stringCalculatorSentence); | ||
| } |
There was a problem hiding this comment.
계산기가 하나의 계산기 자원을 상태로 가지도록 구현하셨네요 !
만약 계산기가 또 다른 자원에 대한 계산을 해야하는 상황이라면 어떻게 해야할까요?
현재 상태에서는 하나의 계산기 객체가 하나의 자원만을 계산할 수 있는데요.
여러 개의 자원에 대해 계산할 수 있도록 개선해보면 좋을 것 같습니다 :)
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class CalculatorResource { |
There was a problem hiding this comment.
CalculatorResource 클래스 네이밍은 추상화가 많이 되어 있는 클래스 명입니다.
클래스 내부를 보지 않으면 어떤 상태를 가지고 있는지 유추하기 힘듭니다. 조금 더 구체적인 클래스 명을 만들어보면 좋을 것 같아요 :)
| private String sentence; | ||
| private String[] sentenceArray; |
There was a problem hiding this comment.
객체가 생성된 이후에 두 변수는 사용되지 않고 있네요! 🤔
그렇다면 계산기 자원클래스에서 sentence와 sentenceArray를 상태로 가지고 있을 필요가 있을까요?
|
안녕하세요 요한님. 베스트 케이스를 분석해보면서 enum을 활용하는 방법과 기능의 확장을 고려하여 계산 재료를 만드는 클래스를 인터페이스로 구현하는 방법을 배울 수 있었습니다. 감사합니다. |
2 participants
안녕하세요 요한님.
앞서 피드백 주셨던 내용과 역할과 책임을 기준으로 객체를 나눠서 사용하는 것을 고민하며 구현해 보았습니다.
문자열 계산기의 로직 자체가 난이도가 낮아 그나마 수월했지만, 객체 지향적인 구조를 설계하는 것은 많은 연습이 필요할 거 같습니다.
바쁘신 와중에도 시간 내 코드 검토해 주셔서 감사드립니다.